Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

EnC - Support record structs #52989

Merged
merged 9 commits into from
May 14, 2021
Merged

Conversation

davidwengier
Copy link
Contributor

@davidwengier davidwengier commented Apr 28, 2021

Fixes #51200

Thank you @jcouv for making this really straightforward 😀

@Youssef1313
Copy link
Member

Fixes #44877

This issue is already closed (it was for records) - probably there is another issue for record structs?

@davidwengier
Copy link
Contributor Author

This issue is already closed (it was for records) - probably there is another issue for record structs?

Thanks, bad copy-paste :)

@davidwengier davidwengier marked this pull request as ready for review April 29, 2021 04:35
@davidwengier davidwengier requested a review from a team as a code owner April 29, 2021 04:35
@davidwengier davidwengier requested a review from tmat April 29, 2021 04:36
@davidwengier davidwengier changed the base branch from features/compiler to main May 4, 2021 21:23
@tmat
Copy link
Member

tmat commented May 8, 2021

We should test that adding fields to record struct reports RudeEditKind.InsertIntoStruct rude edit.

@tmat
Copy link
Member

tmat commented May 8, 2021

Are there any differences in synthesized members between records and record structs?

@davidwengier
Copy link
Contributor Author

davidwengier commented May 10, 2021

Are there any differences in synthesized members between records and record structs?

PrintMembers has different visibility, and there is no Clone$ method or copy constructor, but none of that matters for EnC. If fields were able to be added to structs then we'd at least want to add a test to ensure the semantic edits are right due to the missing copy constructor, but based on how the code is written (ie it enumerates constructors rather than assuming they exist) I don't think it would be an issue.

@davidwengier
Copy link
Contributor Author

@tmat is there anything else for this? any other tests you think are important?

Copy link
Member

@tmat tmat left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:shipit:

@tmat
Copy link
Member

tmat commented May 13, 2021

One more thing we might want to test is changing record to record struct and vice versa. I'm guessing we already correctly report rude edit.

@davidwengier
Copy link
Contributor Author

We do, but I'll add a test anyway.

@davidwengier davidwengier enabled auto-merge (squash) May 13, 2021 22:53
@davidwengier davidwengier merged commit fa5a5bb into dotnet:main May 14, 2021
@ghost ghost added this to the Next milestone May 14, 2021
@davidwengier davidwengier deleted the EnCRecordStruct branch May 14, 2021 00:32
@jcouv
Copy link
Member

jcouv commented May 14, 2021

Thanks!

@RikkiGibson RikkiGibson modified the milestones: Next, 17.0.P2 Jun 29, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

EnC: support for record structs
5 participants